Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

depends on light hermes #49

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

weiyuan-jiang
Copy link
Contributor

This merge requires the merge of GMAO_Shared branch feature/wjiang/#149-add_write_eta which provide the module shared_topo_remap

@weiyuan-jiang weiyuan-jiang requested a review from a team as a code owner March 12, 2020 02:58
AdvCore_GridCompMod.F90 Outdated Show resolved Hide resolved
@@ -7578,7 +7579,14 @@ subroutine Coldstart(gc, import, export, clock, rc)
bk_is_missing = .true.
endif

if (ak_is_missing .or. bk_is_missing) call set_eta(km, ls, ptop, pint, AK, BK)
if (ak_is_missing .or. bk_is_missing) then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates logic above. It should be moved into an internal procedure and then just called from the two locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interfaces are different. I am not sure how to avoid the logic. Since it is already moved to init(), it is not a big issue.

@@ -326,7 +330,11 @@ program interp_restarts

allocate ( r8_ak(npz+1) )
allocate ( r8_bk(npz+1) )
call set_eta(npz,ks,ptop,pint,r8_ak,r8_bk)
if (trim(eta_rc_file) == 'None') then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and duplicated again here ...

Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly little things, but the read during the run method bothers me.

Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - missed that there were two grid comps that needed the change.

Still might be worth refactoring to a shared procedure, but ... harder now.

Copy link
Contributor Author

@weiyuan-jiang weiyuan-jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tclune , Your comments are addressed . @atrayano, You are welcome to make comments

Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice.

If there were any changes, I might ask that you modify the name of the internal state. Conventionally these are named after the component itself, though I think your name is much clearer. So please leave it as is, but be prepared to switch it if anyone asks.

@mathomp4 mathomp4 changed the base branch from master to main June 24, 2020 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants